Repository navigation
Conversation
… §2.3.1) RFC 6749 §2.3.1 requires the client identifier and the client secret to be form-urlencoded (Appendix B) before they are joined with ":" and base64-encoded into the Authorization header; a conformant token endpoint splits the decoded value on the first ":" and form-decodes both halves. Sending them raw meant a secret containing "+" arrived as a space, "%" broke the decoding, a ":" inside the client identifier moved the split, and any character outside Latin-1 made btoa throw before the request was sent. Credentials made only of unreserved characters are byte-identical on the wire.
🦋 Changeset detectedLatest commit: 5378658 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
koriyoshi2041
left a comment
koriyoshi2041
left a comment
There was a problem hiding this comment.
Validated exact head b4837e8a against the RFC 6749 §2.3.1 boundary. The focused auth matrix passes 318/318, client typecheck and formatting pass, and reverting only the encoding call makes all four new regressions fail (reserved + becomes a space, raw delimiters are ambiguous, and non-Latin-1 credentials throw before fetch).
The implementation also preserves the existing wire bytes for unreserved ASCII credentials while making both halves independently form-decodable. I found no correctness issue in the changed path.
claude
Bot
added
the
v2
chrikrah
left a comment
chrikrah
left a comment
There was a problem hiding this comment.
@gdut4140 This is mergeable once formUrlEncode percent-encodes the way the Python SDK does, so ~ stays as it is. The description says ~ becomes %7E, while the changeset says unreserved-only credentials are byte-identical, and only one of those can be true.
non-blocking: URLSearchParams sends ~ as %7E, which is RFC-correct but not what servers see today. Entra client secrets carry a Q~ in their fixed format, Entra lists client_secret_basic, and selectClientAuthMethod prefers basic whenever it is listed (auth.ts:782). Since #1594, servers that omit the list land on basic too. Any server that compares the raw password starts rejecting those secrets on a patch release. mcp 2.3.0 builds the same header with quote(value, safe="") (mcp/client/auth/oauth2.py:263), which leaves ~ alone:
return encodeURIComponent(value).replace(/[!'()*]/g, c => `%${c.charCodeAt(0).toString(16).toUpperCase()}`);$ node p3039.mjs # what reaches the server, per half
secret this PR decodes raw equal | quote style decodes raw equal
Abc8Q~xYz.-_Q Abc8Q%7ExYz.-_Q true false | Abc8Q~xYz.-_Q true true
a+b/c= a%2Bb%2Fc%3D true false | a%2Bb%2Fc%3D true false
two words two+words true false | two%20words true false
$ npx vitest run test/client/authBasicEncoding.test.ts test/client/auth.test.ts test/client/authExtensions.test.ts # packages/client at b4837e8, Node 22.23.3
Tests 318 passed (318)
# auth.ts from 9bc40e9, tests kept: 4 failed (all of authBasicEncoding.test.ts)
# quote-style encoder: 1 failed, 317 passed, only the exact-string assertion ("client+id" vs "client%20id")
# not run: a live Entra or other third-party token endpoint
@felixweinberger, my pick is the quote-style encoder, at the cost of one changed test assertion. Should the changeset state that ~ changes if URLSearchParams stays? That line is what users read in the release notes.
|
Thanks — switched to the quote-style encoder: function percentEncode(value: string): string {
return encodeURIComponent(value).replace(/[!'()*]/g, c => `%${c.charCodeAt(0).toString(16).toUpperCase()}`);
}
Validation: |
gdut4140
changed the title
…Python SDK The encoder now matches the Python SDK's `quote(value, safe="")`: each half is RFC 3986 percent-encoded, so `~` stays raw (Entra client secrets carry a fixed `Q~` prefix) while a space becomes `%20` and `!'()*` are escaped. The test pins `Abc8Q~xYz.-_Q` through both the wire form and a form-decoding recovery, and the changeset now states that unreserved characters, `~` included, stay byte-identical.
675d8a1 to
5378658
Compare
Summary
applyBasicAuthbase64-encoded the client id and secret as they were. RFC 6749 §2.3.1 requires each half to be encoded before they are joined with:, and a token endpoint splits the decoded value on the first:and decodes both halves. The encoder is the RFC 3986 percent-encoding the Python SDK uses (quote(value, safe="")), so~stays raw while spaces,+,%, and a:inside a client identifier are escaped. Credentials containing those characters were therefore sent as different credentials, andbtoathrew outright for anything outside Latin-1, aborting the token request before it left the process:cid/a+ba ba+bclient id/sec:ret+with=specialclient, passwordid:sec:ret with=specialcid/p€ssInvalidCharacterErrorfrombtoa, no request sentp€ssCredentials made only of unreserved characters, including
~, are byte-identical on the wire, so nothing that worked changes. Existing secrets such as Entra'sQ~…keep their exact bytes.Changes
packages/client/src/client/auth.ts: a smallpercentEncodehelper matching the Python SDK'squote(value, safe="")(encodeURIComponentplus!'()*), applied to both halves.~stays raw and a space becomes%20.packages/client/test/client/authBasicEncoding.test.ts: four unit tests assert what a conformant server recovers, including that~is left unencoded; a fifth drives a realexchangeAuthorizationagainst a recordingfetch, so the header on the token request itself is covered..changeset/client-secret-basic-form-encoding.md.Verification
npx vitest run test/client/authBasicEncoding.test.ts— 5 passed.main— the four encoding tests fail; the~guard passes there because rawbtoaalready leaves~alone, and it fails with a form encoder (%7E).vitest run test/client/auth.test.ts test/client/authExtensions.test.ts— 317 passed; the twoprivate_key_jwtcases need--testTimeout=60000on this machine (RSA key generation exceeds vitest's 5 s default).packages/clientsuite — 990 passed, 12 skipped. The only failing files are the threespawn pnpm ENOENTbuild-dependent ones, which fail identically without this change.pnpm --filter @modelcontextprotocol/client run typecheckclean,eslint src/client/auth.tsclean,prettier --checkclean on the changed files.Notes
v1.xcarries the same code (src/client/auth.ts:358). Happy to send the equivalent change to that branch too if you'd like it there.AI disclosure
Per the organization's AI contribution policy: this PR was prepared with Claude Code (an AI coding assistant) under my direction — it located the encoding deviation and drafted the code and tests. I reviewed the change and the evidence above, the reproduction, tests and suite runs were executed locally in my environment, and I can explain why the change is correct: §2.3.1 is explicit that both halves are encoded, and the tests check what a conformant server recovers rather than only the string we emit.